Skip to content

fix: change es version check to support elasticsearch 8+#4512

Closed
kjschnei001 wants to merge 5 commits intojaegertracing:mainfrom
kjschnei001:es8
Closed

fix: change es version check to support elasticsearch 8+#4512
kjschnei001 wants to merge 5 commits intojaegertracing:mainfrom
kjschnei001:es8

Conversation

@kjschnei001
Copy link

@kjschnei001 kjschnei001 commented Jun 6, 2023

Which problem is this PR solving?

  • Allows for writing of spans to a v8 elasticsearch index.
  • I am utilizing the library only for writing spans to an elasticsearch instance, so I can't confirm that these changes are sufficient for elasticsearch 8+ support in jaeger, but I do know that the _type field is incompatible in elasticsearch v8, and removing it allows me to successfully write to an index in elasticsearch v8.
  • Even if it doesn't fully satisfy the requirements for v8 support, it does feel like a very safe change to make, as it doesn't modify the logic for v7 and lower versions of elasticsearch.

Short description of the changes

  • This PR modifies the version check for elasticsearch to apply v7 logic to any version equal to or greater than v7.

@kjschnei001 kjschnei001 requested a review from a team as a code owner June 6, 2023 20:26
@kjschnei001 kjschnei001 requested a review from yurishkuro June 6, 2023 20:26
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (f282d9b) 97.03% compared to head (ff35d00) 97.04%.

❗ Current head ff35d00 differs from pull request most recent head 93f3151. Consider uploading reports for the commit 93f3151 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4512      +/-   ##
==========================================
+ Coverage   97.03%   97.04%   +0.01%     
==========================================
  Files         301      300       -1     
  Lines       17817    17813       -4     
==========================================
- Hits        17289    17287       -2     
+ Misses        423      421       -2     
  Partials      105      105              
Flag Coverage Δ
unittests 97.04% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yurishkuro
Copy link
Member

The change needs to be demonstrated with CI running successfully against v8. I added it to the matrix, but I think there is some infra issue where a started DB is not recognized by our script, needs investigation.

@kjschnei001 kjschnei001 force-pushed the es8 branch 2 times, most recently from a46720f to 66e5693 Compare June 29, 2023 17:09
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
Signed-off-by: KevinSchneider <kevin.schneider@target.com>
@yurishkuro
Copy link
Member

I think this is already fixed, we have CI running for esv8. Closing.

@yurishkuro yurishkuro closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants